-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix location of rosdep cache dir #109
Conversation
src/catkin_lint/ros.py
Outdated
# Python 3 | ||
from io import StringIO | ||
stdout = sys.stdout | ||
sys.stdout = StringIO() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to, but that's not available in Python 2.7 :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still supporting Python 2? 😲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty well, actually :) Melodic isn't dead yet =)
But not in catkin_lint_cmake...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I decided to not support Python 2 in a project I created significant time past the EOL of Python 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's completely okay you decided to abandon Melodic. And that's why I wanted to pull a (simpler) CMake support directly into catkin_lint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no clue whether this entire hassle is really needed for ONLY the ROS buildfarm.
Would it not be an better/simpler option to expose the use_rosdep
.
Well, the buildfarm actually checks for the dependency existence in other ways, so the check from catkin_lint is redundant. However, if you test locally before releasing, the check from catkin_lint is very useful. I haven't yet found a way to reliably detect whether the build is running on ROS buildfarm or not, so it is difficult to make a conditional disabling the checks on buildfarm. |
I think having a way of detecting it, is the only way to get things working, isn't it? How do you otherwise determine the custom paths which you need to provide to |
The best (still indirect) way I found is checking whether the rosdep cache folder exists. If not, then it is probably the buildfarm or some other weird environment.
When using the functionality added in this PR, you won't determine them, you will specify them. Either in So the proposed solution would not introduce anything that would be buildfarm-specific - it would be a general solution for all environments without initialized rosdep database (which, maybe, is only the buildfarm, though). I like this much more than some hacks, e.g. examining hostname and guessing whether it is one of the buildfarm hosts, or checking for the |
I think this is totally fine, when done in a wrapper, CMake macro, etc.. |
This is probably where we won't agree... However, can you identify what exactly is the "hassle" you don't like about this PR? Is it adding the 4 arguments? Alternatively, all of them could be squeezed in a single one, say |
I also try to "attack" this problem from the other side - rosdep+buildfarm : ros-infrastructure/ros_buildfarm#923 (comment) . But in my view, the chances of those changes to be merged are not very high... |
Turns out this is actually not that difficult to detect for |
See #109 for the corresponding discussion
@roehling Detecting the error condition is good, but I'd still rather get a functioning catkin_lint. With the change you did, catkin_lint would still fail on the buildfarm... |
Why do you think that? |
Ahh, right, you didn't change the exit code. But still, being able to check the packages would be even better :) |
Signed-off-by: Martin Pecka <[email protected]>
@roehling I see you were faster than me with integrating the changes :) There is, however, a small bit that would be better fixed. Rosdep uses several cache folders - so far the sources cache and meta cache. catkin_lint has so far only "depended" on the location of the sources cache, but it is possible that there would be other cache directories added in the future, or that it will start being important to also point to a proper meta cache location. Fortunately, both sources cache and meta cache reside in a common parent directory. It'd seem to me more future-proof to configure this parent directory as the base for all caches. And that's also the same interpretation that I also see you started refereing to |
Thanks, merged. Regarding the documentaiton: I chose my wording carefully that the command line option has the same effect as setting |
Yes, that makes perfect sense. Do you actually plan a release into Bionic and Focal anytime soon? I see the last version released there is 1.6.15... |
I have no write access to the Open Robotics repository. I open pull requests, and sometimes they get merged. |
I was wondering where these updates happen as I haven't found it on rosdistro. Thanks :) |
catkin_lint internally uses rosdep to check for existence of packages mentioned in package.xml. When running tests on ROS buildfarm, rosdep database is unfortunately not inited or updated, so any rosdep lookups fail. And there is no way to tell the buildfarm to init and update the db before running tests.
So currently, any code that wants to use catkin_lint in
run_tests
and should be released via ROS buildfarm, needs to do some non-trivial checks to see if there is a rosdep database cache, and in case there isn't, catkin_lint has to be disabled (or at least checking package existence should be disabled via a flag).One way around this limitation is using a custom (non-root writable) location for the rosdep db and cache and downloading the cache files again every time the tests are run (this can be later optimized via CI cache storage, if the folder name is predictable).
So this PR is a first step towards making it possible to use catkin_lint on ROS buildfarm. The other part would be implemented in the CMake calling code (and I'm thinking about submitting another PR with the CMake support similar to https://github.com/tue-robotics/catkin_lint_cmake).